Repository navigation
Conversation
Copilot
AI
changed the title
[WIP] Fix isSafeCommandArgument to account for delegating launchers
Java: detect command injection through delegating launchers
Oct 14, 2026
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
owen-mc
previously requested changes
Oct 14, 2026
Contributor
|
@copilot Please address the three comments on this PR. |
Contributor
Author
Addressed all three comments in edb67fe and 866d93b. Launcher handling now preserves safe arguments after fixed non-shell commands, recognizes the missing wrappers, and includes GOOD cases for literals, lists, and mutable arrays. Added safeguards and regressions for unresolved commands and mutated aliases. All four CWE-078 tests pass; final read-only review found no issues. The automated security scan was skipped because its database was too large. |
Copilot stopped reviewing on behalf of
owen-mc due to an error
October 14, 2026 13:24
Contributor
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
6 open findings
forallquantifiesSsaDefinition defbut then unconditionally castsdefto… · Newexists(...)is used with an expression (exists(this.getArgument(...))), which is not valid QL… · NewcommandOperandre-binds thelauncherparameter usingthis.getFixedArgument(_), which can… · Newexists(...)is used with an expression (exists(this.getArgument(...))), which is not valid QL… · NewisShellaccounts for Windows-style.exesuffixes, butisDelegatingLauncherdoes not. This… · NewunknownIndex[arg.length()]can throwArrayIndexOutOfBoundsExceptionfor most inputs (since the… · New
What changed in this PR
Updates the Java CWE-078 CodeQL modeling and tests to better distinguish delegating “launcher” executables (for example env, sudo, timeout) from shells, improving command-injection and relative-path detection accuracy.
Changes:
- Extend
isSafeCommandArgumentto handle delegating launchers by analyzing fixed command prefixes across arrays/lists/mutable arrays. - Add new test cases covering launcher delegation scenarios and update expected query outputs accordingly.
- Introduce new argument-sequence abstraction (
CommandArguments) and supporting utilities for alias/escape tracking.
| File | Description |
|---|---|
| java/ql/test/query-tests/security/CWE-078/Test.java | Adds new ProcessBuilder test cases for delegating launchers, argument mutation/aliasing, and fixed-vs-tainted scenarios. |
| java/ql/test/query-tests/security/CWE-078/ExecTainted.expected | Updates expected taint-tracking results to include new test cases and shifted line numbers. |
| java/ql/test/query-tests/security/CWE-078/ExecRelative.expected | Updates expected relative-path command results for newly added launcher cases and shifted line numbers. |
| java/ql/lib/semmle/code/java/security/CommandArguments.qll | Implements launcher-aware safe-argument logic, including prefix inspection and container escape tracking. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
|
@copilot Address review comments. |
Contributor
Author
owen-mc
force-pushed
the
copilot/fix-is-safe-command-argument
branch
3 times, most recently
from
October 16, 2026 13:58
4edf3dd to
f1d360c
Compare
Document supported simple forms and known limitations with inline expectations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
owen-mc
force-pushed
the
copilot/fix-is-safe-command-argument
branch
from
October 16, 2026 16:13
f1d360c to
f627764
Compare
owen-mc
force-pushed
the
copilot/fix-is-safe-command-argument
branch
from
October 16, 2026 18:19
f627764 to
78f0ba2
Compare
Recognize fixed command positions without parsing launcher options. Only sanitize later arguments when the delegated command is one unchanged string literal. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
owen-mc
force-pushed
the
copilot/fix-is-safe-command-argument
branch
from
October 16, 2026 18:22
78f0ba2 to
8f59bbb
Compare
owen-mc
marked this pull request as ready for review
October 17, 2026 09:20
owen-mc
dismissed
their stale review
October 17, 2026 09:22
I've changed my mind about supporting lots and lots of delegating launchers. Better to keep it simple.
owen-mc
removed their request for review
October 20, 2026 10:02
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Improves
java/command-line-injectionby detecting tainted commands passed through simple delegating launchers such asenv,sudo, andtimeout.The change:
Launcher options and environment assignments are intentionally left unsupported to keep the implementation simple and conservative.
--
I originally set Copilot to work on this, but it got bogged down in trying to make everything precise. In this case, we just want to see if we can do better than we are currently doing, while keeping things relatively simple. I have tried to strike the right balance in this PR.